Conversation
amomchilov
force-pushed
the
amomchilov/memberof
branch
2 times, most recently
from
529652b to
ba6d3b5
Compare
kddnewton
left a comment
kddnewton
left a comment
There was a problem hiding this comment.
A couple comments. Also it seems like sometimes we have memberof pm_*_t and sometimes we have memberof pm_*. I think I would prefer to use the typedef.
include/prism.h
Outdated
| * The prism version and the serialization format. | ||
| * | ||
| * @returns The prism version as a constant string. | ||
| * \public \memberof pm_parser |
There was a problem hiding this comment.
I would prefer that this not be a member of parser, since it does not accept it as a parameter/doesn't correspond to a parser.
|
|
||
| /** | ||
| * This function is used in pm_parse_stream to retrieve a line of input from a | ||
| * This function is used in pm_parse_stream() to retrieve a line of input from a |
There was a problem hiding this comment.
| * This function is used in pm_parse_stream() to retrieve a line of input from a | |
| * This function is used in pm_parse_stream to retrieve a line of input from a |
There was a problem hiding this comment.
The () makes this into link to the pm_parse_stream() function that uses this callback.
| * This function is used in pm_parse_stream() to retrieve a line of input from a | ||
| * stream. It closely mirrors that of fgets so that fgets can be used as the | ||
| * default implementation. | ||
| * \public \memberof pm_parser |
There was a problem hiding this comment.
This shouldn't be marked as a member of parser, as it's a user-defined callback that does not correspond to a parser.
There was a problem hiding this comment.
It's still strongly related to the parser (similar to a type alias or nested interface defined in a Ruby class), and renders nicely on the same page under "Public types":
WDYT?
There was a problem hiding this comment.
Could you use \relates instead?
include/prism/node.h
Outdated
| * @param node The root node to start visiting from. | ||
| * @param visitor The callback to call for each node in the subtree. | ||
| * @param data An opaque pointer that is passed to the visitor callback. | ||
| * \public \memberof pm_node |
There was a problem hiding this comment.
This one I disagree with, as it's more related to the visitor (the function callback). So I would exclude this.
There was a problem hiding this comment.
What do you think about switching it to "related"?
It'll still help discoverability, since someone looking at this API would probably look in pm_node to start
include/prism/node.h
Outdated
| * @param node The node to visit the children of. | ||
| * @param visitor The callback to call for each child node. | ||
| * @param data An opaque pointer that is passed to the visitor callback. | ||
| * \public \memberof pm_node |
There was a problem hiding this comment.
Same here, this is more related to the visitor, so I don't think this should be associated with a particular node.
amomchilov
force-pushed
the
amomchilov/memberof
branch
2 times, most recently
from
6f947bd to
3f77637
Compare
amomchilov
force-pushed
the
amomchilov/memberof
branch
from
3f77637 to
63f4205
Compare
|
@kddnewton Ready for re-review |
| * This function is used in pm_parse_stream() to retrieve a line of input from a | ||
| * stream. It closely mirrors that of fgets so that fgets can be used as the | ||
| * default implementation. | ||
| * \public \memberof pm_parser |
There was a problem hiding this comment.
Could you use \relates instead?
| * @param parser The parser to serialize. | ||
| * @param node The node to serialize. | ||
| * @param buffer The buffer to serialize to. | ||
| * \public \memberof pm_node |
There was a problem hiding this comment.
Why are we pointing to pm_buffer_t in the previous one and pm_node instead of pm_node_t in this one?
| * @param size The size of the source. | ||
| * @param data The optional data to pass to the parser. | ||
| * @return True if the source parses without errors or warnings. | ||
| * \public \memberof pm_parser |
There was a problem hiding this comment.
Same question about pm_parser_t vs. pm_parser?
| * | ||
| * @param list The list to append to. | ||
| * @param node The node to append. | ||
| * \private \memberof pm_node_list |
There was a problem hiding this comment.
Why are this one and the following ones private?
| * @param node The root node to start visiting from. | ||
| * @param visitor The callback to call for each node in the subtree. | ||
| * @param data An opaque pointer that is passed to the visitor callback. | ||
| * \public \related pm_node |
| * @param node The node to visit the children of. | ||
| * @param visitor The callback to call for each child node. | ||
| * @param data An opaque pointer that is passed to the visitor callback. | ||
| * \public \related pm_node |
|
Superseded by #3641 |
3 participants
Doxygen's
\memberofcommand lets you associate C functions to types, to make them render as if they were actual methods. This is in a similar spirit to\extends, which we use to simulate inheritance frompm_node.Demo:
It's possible to hide the
PRISM_EXPORTED_FUNCTIONmacro spam from these return types, but I'll do that in a separate PR.